Add code coverage disable check to GitHub workflows #2701
Conversation
|
Warning Rate limit exceeded@im-vedant has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a comprehensive script Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/code_coverage_disable_check.py (4)
37-37: Consider enhancing the regex pattern for better precision.The current pattern could be split into separate patterns for inline and block comments for better maintainability and precision.
- code_coverage_disable_pattern = re.compile(r'(?://\s*istanbul\s+ignore(?:-next-line|-line)?|/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/)', re.IGNORECASE) + INLINE_IGNORE_PATTERN = re.compile(r'//\s*istanbul\s+ignore(?:-next-line|-line)?', re.IGNORECASE) + BLOCK_IGNORE_PATTERN = re.compile(r'/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/', re.IGNORECASE) + return bool(INLINE_IGNORE_PATTERN.search(content) or BLOCK_IGNORE_PATTERN.search(content))
63-63: Fix unused loop variable.The
dirsvariable inos.walkis unused.- for root, dirs, files in os.walk(directory): + for root, _dirs, files in os.walk(directory):🧰 Tools
🪛 Ruff (0.8.2)
63-63: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
71-74: Consider refactoring duplicate code coverage check.The setup.ts check duplicates the logic from
has_code_coverage_disable. Consider reusing the existing logic in a more modular way.- setup_path = os.path.join(directory, 'setup.ts') - if os.path.exists(setup_path) and has_code_coverage_disable(setup_path): - print(f'Setup file {setup_path} contains code coverage disable statement.') - code_coverage_found = True + def check_specific_file(filepath, file_type): + if os.path.exists(filepath) and has_code_coverage_disable(filepath): + print(f'{file_type} file {filepath} contains code coverage disable statement.') + return True + return False + + setup_path = os.path.join(directory, 'setup.ts') + code_coverage_found = check_specific_file(setup_path, 'Setup') or code_coverage_found
94-120: Consider enhancing the main function for better testability.While the implementation is solid, consider:
- Separating the execution logic from the main function
- Adding more detailed error messages
+def run_coverage_check(directories): + """Execute the coverage check logic. + + Args: + directories (list): List of directories to check. + + Returns: + bool: True if successful, False if issues found. + """ + print("Checking directories:", directories) + code_coverage_found = check_code_coverage(directories) + + if code_coverage_found: + print("ERROR: Code coverage disable statements found in the following files.") + return False + + print("SUCCESS: No code coverage disable statements found.") + return True + def main(): """Execute the script's main functionality.""" args = arg_parser_resolver() - print("Directories to check: ", args.directory) - - # Check code coverage in the specified directory - code_coverage_found = check_code_coverage(args.directory) - - if code_coverage_found: - print("Code coverage disable check failed. Exiting with error.") + if not run_coverage_check(args.directory): sys.exit(1) - - print("Code coverage disable check completed successfully.").github/workflows/pull-request.yml (1)
190-205: Fix trailing whitespace and consider job dependencies.The job configuration looks good but has a minor formatting issue and could benefit from explicit dependencies.
- Remove trailing whitespace after line 201
- Consider adding this job as a dependency for the Test-Application job, similar to Check-ESlint-Disable
Check-Code-Coverage-Disable: name: Check for code coverage disable runs-on: ubuntu-latest steps: - name: Checkout code uses: actions/checkout@v4 - name: Set up Python uses: actions/setup-python@v5 with: - python-version: 3.9 + python-version: 3.9 - name: Run Python script run: | python .github/workflows/code_coverage_disable_check.py🧰 Tools
🪛 yamllint (1.35.1)
[error] 201-201: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 201-201: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
63-63: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (2)
.github/workflows/code_coverage_disable_check.py (1)
78-92: LGTM! Well-structured argument parser.
The argument parser is well-implemented with clear documentation and appropriate defaults.
.github/workflows/pull-request.yml (1)
205-205: 🛠️ Refactor suggestion
Consider adding job dependency.
The Test-Application job should wait for this check to complete.
Test-Application:
name: Test Application
runs-on: ubuntu-latest
- needs: [Code-Quality-Checks, Check-ESlint-Disable]
+ needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]Likely invalid or redundant comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2701 +/- ##
=====================================================
+ Coverage 31.32% 88.93% +57.61%
=====================================================
Files 299 320 +21
Lines 7429 8290 +861
Branches 1624 1813 +189
=====================================================
+ Hits 2327 7373 +5046
+ Misses 4897 692 -4205
- Partials 205 225 +20 ☔ View full report in Codecov by Sentry. |
|
@palisadoes eslint_disable_check.py does not check for multi-line comments used to disable ESLint, and it cannot be run for multiple directories using the CLI. Can I fix these in this pr? |
|
…d documentation standards.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/eslint_disable_check.py (1)
50-56:⚠️ Potential issueEnhance error handling for CI environment
In a CI environment, it would be more useful to fail fast rather than silently continuing when file errors occur.
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1).github/workflows/code_coverage_disable_check.py (1)
49-57:⚠️ Potential issueEnhance error handling for CI environment
For consistency with the eslint checker, improve error handling to fail fast in CI environment.
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1)
🧹 Nitpick comments (4)
.github/workflows/eslint_disable_check.py (2)
40-44: Consider using verbose regex mode for better readabilityThe regex pattern is complex and could benefit from using verbose mode (
re.VERBOSE) to improve maintainability.eslint_disable_pattern = re.compile( - r"""\/\/\s*eslint-disable(?:-next-line - |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", - re.IGNORECASE, + r""" + (?x) # Enable verbose mode + (?: + \/\/\s* # Single line comment + eslint-disable # eslint-disable + (?:-next-line|-line)? # Optional modifiers + [^\n]* # Rest of the line + | # OR + \/\*\s* # Multi-line comment start + eslint-disable # eslint-disable + [^\*]* # Content + \*\/ # Comment end + ) + """, + re.IGNORECASE,
94-100: Consider extracting setup file check to a shared utilityThe setup file check logic is duplicated in both scripts. Consider extracting it to a shared utility function.
Create a new file
.github/workflows/utils.py:def check_setup_file(directory, check_func, message_prefix): """Check setup.ts file in the given directory. Args: directory (str): Directory containing setup.ts check_func (callable): Function to check the file message_prefix (str): Prefix for the error message Returns: bool: True if issue found, False otherwise """ setup_path = os.path.join(directory, "setup.ts") if os.path.exists(setup_path) and check_func(setup_path): print(f"{message_prefix} {setup_path}") return True return False.github/workflows/code_coverage_disable_check.py (2)
39-43: Consider using verbose regex mode for better readabilitySimilar to the eslint checker, the regex pattern would be more maintainable using verbose mode.
code_coverage_disable_pattern = re.compile( - r"""(?://\s*istanbul\s+ignore(?:-next-line|-line)? - |/\*\s*istanbul\s+ignore\s*(?:next|-line)\s*\*/)""", - re.IGNORECASE, + r""" + (?x) # Enable verbose mode + (?: + //\s* # Single line comment + istanbul\s+ignore # istanbul ignore + (?:-next-line|-line)? # Optional modifiers + | # OR + /\*\s* # Multi-line comment start + istanbul\s+ignore # istanbul ignore + \s*(?:next|-line) # Modifiers + \s*\*/ # Comment end + ) + """, + re.IGNORECASE,
1-155: Consider consolidating the checker scriptsBoth scripts share very similar structure and patterns. Consider consolidating them into a single, configurable checker script to reduce code duplication and improve maintainability.
Create a new file
.github/workflows/code_checker.pythat can handle both eslint and coverage checks:class CodeChecker: def __init__(self, check_type): self.patterns = { 'eslint': r'...', # eslint pattern 'coverage': r'...' # coverage pattern } self.check_type = check_type def check_files(self, directories): """Generic file checking logic""" pass # Usage: # python code_checker.py --type eslint --directory src/ # python code_checker.py --type coverage --directory src/🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/eslint_disable_check.py(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/eslint_disable_check.py (3)
40-44: Add documentation for the regex pattern.The regex pattern is complex and would benefit from inline documentation explaining each part of the pattern.
Consider adding a comment explaining the pattern:
eslint_disable_pattern = re.compile( + # Matches both single-line comments (// eslint-disable...) + # and multi-line comments (/* eslint-disable... */) r"""\/\/\s*eslint-disable(?:-next-line |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", re.IGNORECASE, )
73-93: Simplify nested loops and fix string formatting.The nested loops could be simplified, and the print statements have inconsistent formatting.
Consider this refactoring:
for directory in directories: if not os.path.exists(directory): - print( - f"""Error: The specified directory '{directory}' does not - exist.""" - ) + print(f"Error: The specified directory '{directory}' does not exist.") sys.exit(1) for root, _, files in os.walk(directory): - for file_name in files: - if ( - file_name.endswith(".tsx") - and not file_name.endswith(".test.tsx") - ): - file_path = os.path.join(root, file_name) - if has_eslint_disable(file_path): - print( - f"""File {file_path} contains eslint-disable - statement.""" - ) - eslint_found = True + tsx_files = [f for f in files if f.endswith(".tsx") and not f.endswith(".test.tsx")] + for file_name in tsx_files: + file_path = os.path.join(root, file_name) + if has_eslint_disable(file_path): + print(f"File {file_path} contains eslint-disable statement.") + eslint_found = True
94-100: Fix string formatting in setup.ts check.The print statement for setup.ts has inconsistent formatting.
setup_path = os.path.join(directory, "setup.ts") if os.path.exists(setup_path) and has_eslint_disable(setup_path): - print( - f"""Setup file {setup_path} contains eslint-disable - statement.""" - ) + print(f"Setup file {setup_path} contains eslint-disable statement.") eslint_found = True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/eslint_disable_check.py(5 hunks)
🔇 Additional comments (4)
.github/workflows/eslint_disable_check.py (4)
7-8: LGTM! Documentation updates are clear and accurate.
The documentation has been properly updated to reflect the script's functionality and compliance standards.
Also applies to: 20-20
50-56: LGTM! Improved error handling with specific exceptions.
The error handling has been enhanced with specific cases for FileNotFoundError and PermissionError, providing better feedback.
115-118: LGTM! CLI argument handling properly supports multiple directories.
The implementation of multiple directory support is well done with clear help messages.
40-44: Verify the regex pattern against various eslint-disable formats.
Let's verify that the regex pattern correctly catches all variations of eslint-disable comments.
✅ Verification successful
The regex pattern correctly matches all eslint-disable comment variations in the codebase
The verification shows that the regex pattern in eslint_disable_check.py correctly matches all variations of eslint-disable comments found in the codebase, including:
- Single-line comments with
eslint-disable-next-line - Multi-line comments with various formats:
/* eslint-disable *//* eslint-disable @typescript-eslint/no-explicit-any *//*eslint-disable*/(without spaces)
The pattern's flexibility in handling whitespace and optional rule specifications makes it robust for the codebase's actual usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test the regex pattern against various eslint-disable formats
# Create a test file with different eslint-disable patterns
cat << 'EOF' > test_eslint.tsx
// eslint-disable-next-line
// eslint-disable-line
// eslint-disable
/* eslint-disable */
/* eslint-disable-next-line */
EOF
# Run the script on the test file
python3 .github/workflows/eslint_disable_check.py --directory .
# Clean up
rm test_eslint.tsx
Length of output: 191
Script:
#!/bin/bash
# Let's verify the regex pattern by searching for actual eslint-disable patterns in the codebase
# This will help us understand if the pattern matches real-world usage
# Search for single-line eslint-disable comments
echo "=== Single-line eslint-disable patterns ==="
rg "//.*eslint-disable.*" -g "!.github/*" --no-heading
echo -e "\n=== Multi-line eslint-disable patterns ==="
# Search for multi-line eslint-disable comments
rg "/\*.*eslint-disable.*\*/" -g "!.github/*" --no-heading
Length of output: 839
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/eslint_disable_check.py (3)
40-44: Improve regex pattern readability and efficiency.The regex pattern could be more readable and maintainable with proper formatting and the use of re.VERBOSE flag.
Consider this improvement:
- eslint_disable_pattern = re.compile( - r"""\/\/\s*eslint-disable(?:-next-line - |-line)?[^\n]*|\/\*\s*eslint-disable[^\*]*\*\/""", - re.IGNORECASE, - ) + eslint_disable_pattern = re.compile( + r''' + # Match single-line comments + \/\/\s*eslint-disable(?:-next-line|-line)?[^\n]* + | + # Match multi-line comments + \/\*\s*eslint-disable[^\*]*\*\/ + ''', + re.IGNORECASE | re.VERBOSE, + )
73-79: Consider checking all directories upfront.Moving the directory existence check outside the main loop would be more efficient and provide better user feedback.
Consider this approach:
+ # Validate all directories upfront + for directory in directories: + if not os.path.exists(directory): + print(f"Error: The specified directory '{directory}' does not exist.") + sys.exit(1) + for directory in directories: - if not os.path.exists(directory): - print( - f"""Error: The specified directory '{directory}' does not - exist.""" - ) - sys.exit(1) for root, _, files in os.walk(directory):
90-93: Fix inconsistent string formatting.The multi-line string literals have inconsistent indentation which could affect readability.
Consider using consistent string formatting:
- print( - f"""File {file_path} contains eslint-disable - statement.""" - ) + print(f"File {file_path} contains eslint-disable statement.") - print( - f"""Setup file {setup_path} contains eslint-disable - statement.""" - ) + print(f"Setup file {setup_path} contains eslint-disable statement.")Also applies to: 98-101
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/eslint_disable_check.py(5 hunks)
🔇 Additional comments (3)
.github/workflows/eslint_disable_check.py (3)
7-8: LGTM! Documentation is comprehensive and follows standards.
The docstring clearly explains the methodology and compliance with Python coding standards, including the addition of Python Black.
Also applies to: 20-20
50-56: LGTM! Error handling is comprehensive.
The error handling is well-structured with specific exceptions for different scenarios, providing clear feedback to users.
117-120: LGTM! CLI argument handling is well implemented.
The implementation correctly supports multiple directories with clear help messages and proper default values.
|
@palisadoes Can you review this pr? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
190-241: Consider caching Python dependenciesTo improve workflow execution time, consider caching pip dependencies using actions/cache.
Add this step after Python setup:
- name: Cache pip dependencies uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('.github/workflows/code_coverage_disable_check.py') }} restore-keys: | ${{ runner.os }}-pip-🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2086:info:11:23: Double quote to prevent globbing and word splitting
(shellcheck)
203-203: shellcheck reported issue in this script: SC2076:warning:14:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:27:38: Double quote to prevent globbing and word splitting
(shellcheck)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 225-225: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
203-203: shellcheck reported issue in this script: SC2086:info:11:23: Double quote to prevent globbing and word splitting
(shellcheck)
203-203: shellcheck reported issue in this script: SC2076:warning:14:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:27:38: Double quote to prevent globbing and word splitting
(shellcheck)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 225-225: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
190-196: Add code coverage check as a dependency for Test-Application job
The Test-Application job should wait for the code coverage check to complete.
197-200: LGTM! Changed files retrieval is properly implemented.
232-241:
Fix script execution and improve error handling
The script execution needs to be improved for security and reliability:
- Fix shell globbing vulnerability
- Add explicit error handling
Apply this diff:
- name: Run Python script
run: |
- # Access the nearest_dirs environment variable
- python .github/workflows/code_coverage_disable_check.py --directory $NEAREST_DIRS
+ if ! python .github/workflows/code_coverage_disable_check.py --directory "${NEAREST_DIRS}"; then
+ echo "Error: Code coverage disable check failed"
+ exit 1
+ fiLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
238-238: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
190-238: Optimize job execution and integrationConsider the following improvements to the job:
- Add conditional execution based on file types (e.g., only run on
.tsxand.tsfiles)- Integrate with other code quality checks by adding it to the
Code-Quality-ChecksjobExample implementation:
Check-Code-Coverage-Disable: name: Check for code coverage disable runs-on: ubuntu-latest + if: | + contains(steps.changed-files.outputs.all_changed_files, '.tsx') || + contains(steps.changed-files.outputs.all_changed_files, '.ts') steps: # ... existing steps ...🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 222-222: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 222-222: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/pull-request.yml (3)
190-193: Add code coverage check as a dependency for Test-Application job
The Test-Application job should wait for the code coverage check to complete before running.
234-237:
Fix shell globbing vulnerability and improve error handling
The Python script execution has several issues that need to be addressed:
- Shell globbing vulnerability in directory argument
- Missing error handling for script execution
Apply this diff to fix the issues:
- name: Run Python script
run: |
# Access the nearest_dirs environment variable
- python .github/workflows/code_coverage_disable_check.py --directory $NEAREST_DIRS
+ if ! python .github/workflows/code_coverage_disable_check.py --directory "${NEAREST_DIRS}"; then
+ echo "Error: Code coverage disable check failed"
+ exit 1
+ fiLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
235-235: shellcheck reported issue in this script: SC2086:info:2:69: Double quote to prevent globbing and word splitting
(shellcheck)
201-227:
Fix shell script security vulnerabilities
The directory identification script has several issues that need to be addressed:
- Unquoted variables leading to word splitting
- Incorrect regex comparison syntax
- Missing input validation
- Undefined
repo_rootvariable used in path construction
Apply this diff to fix the security issues:
- name: Identify nearest changed directories
id: nearest_dirs
run: |
+ if [ -z "${{ steps.changed-files.outputs.all_changed_files }}" ]; then
+ echo "No files changed"
+ exit 0
+ fi
+
# Initialize an empty string to store the nearest directories
nearest_dirs=""
# Loop through each changed file to extract the nearest directory
- for file in ${{ steps.changed-files.outputs.all_changed_files }}
+ IFS=$'\n'
+ for file in $(echo "${{ steps.changed-files.outputs.all_changed_files }}" | tr ',' '\n')
do
# Get the full directory from the file path
folder=$(dirname "$file")
- full_path="$repo_root/$folder"
+ full_path="${folder}"
# Add the folder to the nearest_dirs string if not already present
- if [[ ! " $nearest_dirs " =~ " $full_path " ]]; then
+ if [[ ! " ${nearest_dirs} " =~ [[:space:]]${full_path}[[:space:]] ]]; then
- if [ -z "$nearest_dirs" ]; then
+ if [ -z "${nearest_dirs}" ]; then
- nearest_dirs="$full_path"
+ nearest_dirs="${full_path}"
else
- nearest_dirs="$nearest_dirs,$full_path"
+ nearest_dirs="${nearest_dirs},${full_path}"
fi
fi
done
# Output the nearest changed directories
- echo "Nearest changed directories: $nearest_dirs"
+ echo "Nearest changed directories: ${nearest_dirs}"
# Set the nearest directories as a GitHub environment variable
- echo "NEAREST_DIRS=$nearest_dirs" >> $GITHUB_ENV
+ echo "NEAREST_DIRS=${nearest_dirs}" >> "${GITHUB_ENV}"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
203-203: shellcheck reported issue in this script: SC2076:warning:11:32: Remove quotes from right-hand side of =~ to match as a regex rather than literally
(shellcheck)
203-203: shellcheck reported issue in this script: SC2086:info:24:38: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 222-222: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/setupTests.ts (1)
6-6: Avoid disabling ESLint globally.This directive applies to the entire file and may mask other issues. Rather than disabling ESLint for the entire file, consider disabling specific rules for selected lines where necessary.
.github/workflows/eslint_disable_check.py (2)
75-77: Combine nestedifstatements to improve clarity.The nested checks for
if item.endswith(".ts") or item.endswith(".tsx"):followed by anotherif has_eslint_disable(item):can be combined for simplicity.Apply this diff:
- if item.endswith(".ts") or item.endswith(".tsx"): - if has_eslint_disable(item): - print(f"File {item} contains eslint-disable statement.") - eslint_found = True + if (item.endswith(".ts") or item.endswith(".tsx")) and has_eslint_disable(item): + print(f"File {item} contains eslint-disable statement.") + eslint_found = True🧰 Tools
🪛 Ruff (0.8.2)
75-76: Use a single
ifstatement instead of nestedifstatements(SIM102)
139-139: Consider removing leftover print statements for production code.The
print(files_or_directories)call might be for debugging and could be removed or replaced with a structured logger to reduce console clutter..github/workflows/pull-request.yml (1)
212-212: Remove trailing spaces.This line contains trailing whitespace. Consider removing it to keep the file clean and conform to linting standards.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 212-212: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/eslint_disable_check.py(3 hunks).github/workflows/pull-request.yml(1 hunks)src/setupTests.ts(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 212-212: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
.github/workflows/eslint_disable_check.py
75-76: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
192-193: 🛠️ Refactor suggestion
Double-quote environment variables to prevent word splitting.
When passing file paths to the Python script, wrap the variable in quotes to avoid shell word splitting, especially if any filenames contain spaces.
- python .github/workflows/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }}
+ python .github/workflows/eslint_disable_check.py --files "${{ steps.changed-files.outputs.all_changed_files }}"Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/code_coverage_disable_check.py (1)
28-56:⚠️ Potential issueImprove error handling for CI environment
The current error handling approach silently returns False for all errors, which could mask real issues in a CI environment. This needs to be fixed to ensure proper error propagation.
Apply this diff to improve error handling:
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1).github/workflows/pull-request.yml (2)
181-193:⚠️ Potential issueFix shell globbing vulnerability
The current script execution is vulnerable to shell globbing and word splitting.
- name: Run Python script run: | - python .github/workflows/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} + python .github/workflows/eslint_disable_check.py --files "${{ steps.changed-files.outputs.all_changed_files }}"
194-212:⚠️ Potential issueFix shell globbing vulnerability in code coverage check
The script execution is vulnerable to shell globbing and word splitting.
- name: Run Python script run: | - python .github/workflows/code_coverage_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} + python .github/workflows/code_coverage_disable_check.py --files "${{ steps.changed-files.outputs.all_changed_files }}"🧰 Tools
🪛 yamllint (1.35.1)
[error] 212-212: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (3)
.github/workflows/code_coverage_disable_check.py (2)
99-123: Add input validation for CLI argumentsConsider adding validation to ensure provided paths exist and are accessible.
+def validate_path(path): + """Validate if path exists and is accessible.""" + if not os.path.exists(path): + print(f"ERROR: Path does not exist: {path}") + sys.exit(1) + if not os.access(path, os.R_OK): + print(f"ERROR: Path is not readable: {path}") + sys.exit(1) + return path def arg_parser_resolver(): parser = argparse.ArgumentParser() parser.add_argument( "--directory", type=str, nargs="+", + type=validate_path, default=[os.getcwd()], help="""One or more directories to check for code coverage disable statements (default: current directory).""", ) parser.add_argument( "--files", type=str, nargs="+", + type=validate_path, default=[], help="""One or more files to check directly for code coverage disable statements (default: check directories).""", )
142-143: Remove debug print statementThe print statement on line 143 appears to be for debugging purposes and should be removed.
files_or_dirs = args.files if args.files else args.directory - print(files_or_dirs).github/workflows/pull-request.yml (1)
217-217: Fix YAML formattingAdd a space after the comma in the needs array.
- needs: [Code-Quality-Checks, Check-ESlint-Disable,Check-Code-Coverage-Disable] + needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]🧰 Tools
🪛 yamllint (1.35.1)
[warning] 217-217: too few spaces after comma
(commas)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 212-212: trailing spaces
(trailing-spaces)
[warning] 217-217: too few spaces after comma
(commas)
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
86-92: Use a single if statement instead of nested if statements
(SIM102)
88-92: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (1)
.github/workflows/code_coverage_disable_check.py (1)
1-26: LGTM! Well-documented script with appropriate imports.
The docstring clearly explains the script's purpose and compliance standards. The imports are appropriate for the required functionality.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
|
@palisadoes I have made the necessary changes. I have also added a file to check whether these tests checks the presence of disable statements. Can you review the logic ? Once you review it, I will make the changes so that all python scripts in the PR are compliant with pylint, pycodestyle, pydocstyle, flake8 and black. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
220-220: Fix formatting in the needs array.Add a space after the comma for better readability.
Apply this diff:
- needs: [Code-Quality-Checks, Check-ESlint-Disable,Check-Code-Coverage-Disable] + needs: [Code-Quality-Checks, Check-ESlint-Disable, Check-Code-Coverage-Disable]🧰 Tools
🪛 yamllint (1.35.1)
[warning] 220-220: too few spaces after comma
(commas)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 215-215: trailing spaces
(trailing-spaces)
[warning] 220-220: too few spaces after comma
(commas)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/code_coverage_disable_check.py (1)
49-57: 🛠️ Refactor suggestionEnhance error handling for CI environment.
Similar to eslint_disable_check.py, improve error handling for CI environment.
Apply this diff:
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1)
🧹 Nitpick comments (3)
.github/workflows/eslint_disable_check.py (1)
73-94: Simplify nested conditions for better readability.The nested if statements can be combined using logical operators.
Apply this diff:
for item in files_or_directories: if os.path.isfile(item): - # If it's a file, directly check it - if item.endswith(".ts") or item.endswith(".tsx"): - if has_eslint_disable(item): + # Check if it's a TypeScript file and contains eslint-disable + if (item.endswith(".ts") or item.endswith(".tsx")) and has_eslint_disable(item): print(f"File {item} contains eslint-disable statement.") eslint_found = True🧰 Tools
🪛 Ruff (0.8.2)
76-77: Use a single
ifstatement instead of nestedifstatements(SIM102)
.github/workflows/code_coverage_disable_check.py (2)
40-44: Consider additional istanbul ignore patterns.The current pattern might miss some valid istanbul ignore variations.
Consider updating the pattern to handle additional cases:
code_coverage_disable_pattern = re.compile( - r"""//?\s*istanbul\s+ignore(?:\s+(?:next|-line))?[^\n]*| - /\*\s*istanbul\s+ignore\s+(?:next|-line)\s*\*/""", + r"""(?: + //?\s*istanbul\s+ignore(?:\s+(?:next|-line|\w+))?[^\n]*| + /\*\s*istanbul\s+ignore(?:\s+(?:next|-line|\w+))?\s*\*/ + )""", re.IGNORECASE | re.VERBOSE, )This pattern also captures:
- Function-level ignores (e.g.,
/* istanbul ignore function */)- Custom ignore reasons (e.g.,
// istanbul ignore else - not implemented)
83-86: Simplify print statements.The multi-line print statements can be simplified for better readability.
Apply this diff:
- print( - f"""File {file_path} contains code coverage - disable statement.""" - ) + print(f"File {file_path} contains code coverage disable statement.") - print(f"""File {item} contains code coverage disable - statement.""") + print(f"File {item} contains code coverage disable statement.")Also applies to: 99-100
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/code_coverage_disable_check.py(1 hunks).github/workflows/eslint_disable_check.py(3 hunks)src/setupTests.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/setupTests.ts
🧰 Additional context used
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
88-98: Use a single if statement instead of nested if statements
(SIM102)
90-98: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
.github/workflows/eslint_disable_check.py
76-77: Use a single if statement instead of nested if statements
(SIM102)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (3)
.github/workflows/eslint_disable_check.py (3)
7-21: LGTM: Documentation updates are accurate and complete.
The docstring has been properly updated to reflect the script's enhanced capabilities and compliance standards.
40-44: LGTM: Regex pattern is correctly implemented.
The pattern effectively captures eslint-disable statements in both single-line and multi-line comment formats.
106-120: LGTM: CLI argument handling is well-implemented.
The updated argument parser properly handles multiple files and directories with clear help messages.
| if ( | ||
| item.endswith(".tsx") | ||
| or item.endswith(".ts") | ||
| and not file_name.endswith(".test.tsx") | ||
| and not file_name.endswith(".test.ts") | ||
| and not file_name.endswith(".spec.tsx") | ||
| and not file_name.endswith(".spec.ts") | ||
| ): | ||
| if has_code_coverage_disable(item): | ||
| print(f"""File {item} contains code coverage disable | ||
| statement.""") | ||
| code_coverage_found = True |
There was a problem hiding this comment.
Fix undefined variable in file extension check.
The file_name variable is undefined in this block, causing potential runtime errors.
Apply this diff:
elif os.path.isfile(item):
# If it's a file, check it directly
if (
item.endswith(".tsx")
or item.endswith(".ts")
- and not file_name.endswith(".test.tsx")
- and not file_name.endswith(".test.ts")
- and not file_name.endswith(".spec.tsx")
- and not file_name.endswith(".spec.ts")
+ and not item.endswith(".test.tsx")
+ and not item.endswith(".test.ts")
+ and not item.endswith(".spec.tsx")
+ and not item.endswith(".spec.ts")
):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| item.endswith(".tsx") | |
| or item.endswith(".ts") | |
| and not file_name.endswith(".test.tsx") | |
| and not file_name.endswith(".test.ts") | |
| and not file_name.endswith(".spec.tsx") | |
| and not file_name.endswith(".spec.ts") | |
| ): | |
| if has_code_coverage_disable(item): | |
| print(f"""File {item} contains code coverage disable | |
| statement.""") | |
| code_coverage_found = True | |
| if ( | |
| item.endswith(".tsx") | |
| or item.endswith(".ts") | |
| and not item.endswith(".test.tsx") | |
| and not item.endswith(".test.ts") | |
| and not item.endswith(".spec.tsx") | |
| and not item.endswith(".spec.ts") | |
| ): | |
| if has_code_coverage_disable(item): | |
| print(f"""File {item} contains code coverage disable | |
| statement.""") | |
| code_coverage_found = True |
🧰 Tools
🪛 Ruff (0.8.2)
90-98: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/code_coverage_disable_check.py (1)
49-57:⚠️ Potential issueEnhance error handling for CI environment
In a CI environment, it would be more useful to propagate the error rather than silently returning False.
except FileNotFoundError: - print(f"File not found: {file_path}") - return False + print(f"ERROR: File not found: {file_path}") + sys.exit(1) except PermissionError: - print(f"Permission denied: {file_path}") - return False + print(f"ERROR: Permission denied: {file_path}") + sys.exit(1) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") - return False + print(f"ERROR: Failed to read file {file_path}") + print(f"Cause: {str(e)}") + sys.exit(1)
🧹 Nitpick comments (2)
.github/workflows/code_coverage_disable_check.py (2)
40-44: Improve regex readability with verbose modeThe regex pattern would be more maintainable with verbose mode and comments.
code_coverage_disable_pattern = re.compile( - r"""//?\s*istanbul\s+ignore(?:\s+(?:next|-line))?[^\n]*| - /\*\s*istanbul\s+ignore\s+(?:next|-line)\s*\*/""", - re.IGNORECASE, + r""" + # Match single-line comments with istanbul ignore + //?\s*istanbul\s+ignore(?:\s+(?:next|-line))?[^\n]*| + # Match multi-line comments with istanbul ignore + /\*\s*istanbul\s+ignore\s+(?:next|-line)\s*\*/ + """, + re.IGNORECASE | re.VERBOSE, )
73-110: Refactor to reduce code duplication and improve readabilityThe file extension checking logic is duplicated between directory and file checks. Consider extracting it to a helper function.
+def is_valid_typescript_file(file_name): + """Check if the file is a non-test TypeScript file.""" + return ( + (file_name.endswith(".tsx") or file_name.endswith(".ts")) + and not file_name.endswith(".test.tsx") + and not file_name.endswith(".test.ts") + and not file_name.endswith(".spec.tsx") + and not file_name.endswith(".spec.ts") + ) + def check_code_coverage(files_or_dirs): """Check TypeScript files for code coverage disable statements.""" code_coverage_found = False for item in files_or_dirs: if os.path.isdir(item): for root, _, files in os.walk(item): if "node_modules" in root: continue for file_name in files: - if ( - file_name.endswith(".tsx") - or file_name.endswith(".ts") - and not file_name.endswith(".test.tsx") - and not file_name.endswith(".test.ts") - and not file_name.endswith(".spec.tsx") - and not file_name.endswith(".spec.ts") - ): + if is_valid_typescript_file(file_name): file_path = os.path.join(root, file_name) if has_code_coverage_disable(file_path): print(f"File {file_path} contains code coverage disable statement.") code_coverage_found = True elif os.path.isfile(item): - if ( - item.endswith(".tsx") - or item.endswith(".ts") - and not item.endswith(".test.tsx") - and not item.endswith(".test.ts") - and not item.endswith(".spec.tsx") - and not item.endswith(".spec.ts") - ): + if is_valid_typescript_file(item): if has_code_coverage_disable(item): print(f"File {item} contains code coverage disable statement.") code_coverage_found = True🧰 Tools
🪛 Ruff (0.8.2)
95-105: Use a single
ifstatement instead of nestedifstatements(SIM102)
97-105: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/code_coverage_disable_check.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
.github/workflows/code_coverage_disable_check.py
95-105: Use a single if statement instead of nested if statements
(SIM102)
97-105: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/code_coverage_disable_check.py
[warning] 1-1:
File ignored by default.
🔇 Additional comments (4)
.github/workflows/code_coverage_disable_check.py (4)
1-27: Well-documented module with appropriate imports!
The comprehensive docstring clearly explains the purpose, methodology, and compliance standards. All necessary imports are present and properly organized.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
115-139: Well-implemented argument parsing!
The function provides a clean interface for handling both directory and file inputs with appropriate defaults and help messages.
141-169: Clean and well-structured main execution flow!
The main function is well-documented and handles the execution flow appropriately with proper error handling.
80-87:
Fix incorrect file extension check
The extension check is using item (the directory path) instead of file_name.
if (
- item.endswith(".tsx")
- or item.endswith(".ts")
+ file_name.endswith(".tsx")
+ or file_name.endswith(".ts")
and not file_name.endswith(".test.tsx")
and not file_name.endswith(".test.ts")
and not file_name.endswith(".spec.tsx")
and not file_name.endswith(".spec.ts")
):Likely invalid or redundant comment.
palisadoes
merged commit 7bb67a5
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
New Script:
Added .github/workflows/code_coverage_disable_check.py script.
Recursively scans specified directories for istanbul ignore patterns in .tsx and setup.ts files (excluding .test.tsx).
Supports multiple directories via --directory CLI argument with a default directory option.
GitHub Workflow:
Integrated the code_coverage_disable_check.py into the workflow.
Automatically runs during PR checks to ensure no code coverage disable statements are introduced.
Issue Number:
Fixes #2594
Snapshots/Videos:
Summary by CodeRabbit
New Features
Chores
Tests